Repository navigation
[FEATURE] Declare the restart parameters on extract() and document what a restart costs - #672
Merged
Merged
Conversation
…publish what a restart costs extract() took run_id and run_plan_store only through **kwargs, so they were missing from its signature, its docstring and help(), and they also reached BuildPipeline.create, which has no use for them. They are now named parameters, documented, and passed to the extraction pipeline only. indexing.mdx gains the measured cost of a restart at 100, 500 and 1,000 documents, with the configuration behind each figure and what the figures leave out.
noel-improv
marked this pull request as ready for review
November 5, 2026 22:19
… handler The two runs took 8h 43m and 3h 11m, so the 8m 5s of restarts is 1.5% and 4.2% of them. The figures come from interrupting inside staging, which waits for uploads in flight, so they are slightly optimistic against a killed process. run_id alone does not skip staged documents; the handler has to be the run plan store's staging handler too.
oussamahansal
approved these changes
Nov 9, 2026
…ild() and say auto_tune refuses a run_id extract_and_build() took run_id and run_plan_store only through **kwargs, so they reached BuildPipeline.create as well as the extraction pipeline. The build side passes its kwargs to each transform and nothing there reads either name, so dropping them from the build call changes nothing. They are now named parameters passed to the extraction pipeline only, as on extract(). The extract_and_build() docstring leaves out the staging handler: its handler runs after the build, so there is nothing staged to skip, and a restart there reuses the recorded batch jobs. Both docstrings now say that a run_id with auto_tune=True raises a ValueError, in the docs page's wording, since ExtractionPipeline raises it for either entry point.
acarbonetto
approved these changes
Nov 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
extract()andextract_and_build()acceptedrun_idandrun_plan_storeonly through**kwargs, so a restartable run was missing from their signatures, their docstrings andhelp(). This declares both as named parameters and documents what a restart costs, using the restart benchmark from #590.Changes
LexicalGraphIndex.extract()andextract_and_build()declarerun_idandrun_plan_store(both defaultNone), document them, including theValueErrorwithauto_tune=True, and pass them to the extraction pipeline only. Before this they also reachedBuildPipeline.create, which does not use them.indexing.mdxadds "What a restart costs" to the restart section: per-restart time at 100, 500 and 1,000 documents, the configuration behind each figure, and what the figures leave out.Nonewhen not passed.No feature flag gates restart, so nothing was removed. I searched
lexical-graph/src,docs-site, the examples and everyenv.templateand README for one.Problem
Restart shipped in 3.20, but its parameters were undocumented on the method a caller uses, and there was no published number for what a restart costs.
Related issue (if any): #
Testing
pytest)tests/unit/test_lexical_graph_index.pyandtests/unit/indexing/extract: 411 passed. Withlexical_graph_index.pyreverted, the new tests fail. The docs page was not built locally.Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.